-
Notifications
You must be signed in to change notification settings - Fork 129
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: warn-cacao-near-exp #3164
base: develop
Are you sure you want to change the base?
Conversation
log a warning when a cacao is verified with an expiration within 12h of the covering time event.
@@ -366,6 +367,12 @@ export class StreamUtils { | |||
logEntry.expirationTime | |||
}. Loading the stream with 'sync: SyncOptions.ALWAYS_SYNC' will restore the stream to a usable state, by discarding the invalid commits (this means losing the data from those invalid writes!)` | |||
) | |||
} else if (logEntry.expirationTime && logEntry.expirationTime - timestamp < 12 * 60 * 60) { | |||
logger.warn( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since timestamp
can come from the anchor commit time OR from "now" if the stream isn't actually anchored yet, this log message is a bit misleading, since the message implies the stream is already anchored in all cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
anchorTime=${logEntry.timestamp}
will be null if there is no anchor but I see what you mean about the wording.
@@ -366,6 +367,12 @@ export class StreamUtils { | |||
logEntry.expirationTime | |||
}. Loading the stream with 'sync: SyncOptions.ALWAYS_SYNC' will restore the stream to a usable state, by discarding the invalid commits (this means losing the data from those invalid writes!)` | |||
) | |||
} else if (logEntry.expirationTime && logEntry.expirationTime - timestamp < 12 * 60 * 60) { | |||
logger.warn( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thinking about this more, I'm actually not sure how useful this log message is, and I'm afraid it might be quite noisy. This is because there's nothing that prevents us from doing a write with a CACAO right before it's about to expire. A standard CACAO session is valid for a week by default, but if the user logs in once, then logs in again 6 days, 23 hours, and 59 minutes later, the app will still let them re-use their old session without signing in again. So the mere fact that the CACAO is close to expiring doesn't actually tell us that the stream took longer than expected to be anchored.
I think this log message really needs to be based on the timestamp of when the write was performed, which we only have in the ComposeDB index database.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once we have the 24 hour grace period logging the fact that it is expired tells us basicly the same thing but it is more clear that a client is not refreshing it CACAO sessions aggressively enugh.
log a warning when a cacao is verified with an expiration within 12h of the covering time event.
Description
Include a summary of the change or link to the issue it addresses.
Include relevant motivation, context, brief description and impact of the change(s). List follow-up tasks here.
How Has This Been Tested?
Describe the tests that you ran to verify your changes. Provide instructions for reproduction.
PR checklist
Before submitting this PR, please make sure:
References:
Please list relevant documentation (e.g. tech specs, articles, related work etc.) relevant to this change, and note if the documentation has been updated.